-
Notifications
You must be signed in to change notification settings - Fork 788
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP][history] refactor history client with redirect wrapper #5762
Conversation
0b569e2
to
76b624f
Compare
Codecov Report
Additional details and impacted files
... and 13 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
@@ -950,6 +954,11 @@ func (v *HistoryStartWorkflowExecutionRequest) GetPartitionConfig() (o map[strin | |||
return | |||
} | |||
|
|||
// ToRedirectKey convert request to redirect key | |||
func (v *HistoryStartWorkflowExecutionRequest) ToRedirectKey() RedirectKey { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unit test please :)
@@ -100,3 +102,20 @@ func (pr PeerResolver) GetAllPeers() ([]string, error) { | |||
} | |||
return peers, nil | |||
} | |||
|
|||
// FromRedirectKey resolve the peer from types.RedirectKey by a specific order | |||
func (pr PeerResolver) FromRedirectKey(key types.RedirectKey) (string, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the other PeerResolver.From*
functions on can be made private once all usage is refactored. Is that the plan?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that's a good suggestion
@@ -22,6 +22,7 @@ package history | |||
|
|||
import ( | |||
"context" | |||
"github.com/uber/cadence/common/peerresolver" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make fmt
if key.HostAddress != "" { | ||
return pr.FromHostAddress(key.HostAddress) | ||
} | ||
return "", fmt.Errorf("RedirectKey is not valid: %v", key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's add unit tests
@@ -100,3 +102,20 @@ func (pr PeerResolver) GetAllPeers() ([]string, error) { | |||
} | |||
return peers, nil | |||
} | |||
|
|||
// FromRedirectKey resolve the peer from types.RedirectKey by a specific order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's document the "specific order" so it's more explicit in documentation
no other use cases for the wrapper and might just leave it as it is. Also tests are already covering this. |
What changed?
FromRedirectKey
methodWhy?
simplify history client to make testing easier
How did you test it?
Potential risks
Release notes
Documentation Changes